Skip to content

Skip unsupported file types during source crawl - #303

Open
PuneetPunamiya wants to merge 2 commits into
redhat-data-and-ai:mainfrom
PuneetPunamiya:skip-unsupported-file-types
Open

Skip unsupported file types during source crawl#303
PuneetPunamiya wants to merge 2 commits into
redhat-data-and-ai:mainfrom
PuneetPunamiya:skip-unsupported-file-types

Conversation

@PuneetPunamiya

@PuneetPunamiya PuneetPunamiya commented Jul 31, 2026

Copy link
Copy Markdown
Contributor
  • Only sync Docling-compatible document extensions from S3/GDrive and surface skipped files in the SourceCrawler Ready status message

Summary

  • Source crawler only downloads/stores supported document extensions (aligned with Docling from_formats, excluding image)
  • Unsupported files are skipped (not ingested) and listed in the SourceCrawler Ready condition message
  • Ready stays True when skips occur; real store/API failures still fail reconcile
  • Every discovered file gets a catalog JSON under stages/<crawl>/catalog/<file_id>.json (upserted each reconcile; folders are not catalogued)

Supported file types

  • .pdf
  • .md / .markdown
  • .docx
  • .pptx
  • .html / .htm
  • .csv
  • .xlsx
  • .adoc / .asciidoc
  • .txt
  • .doc (legacy Word)

Not supported yet (skipped)

  • Images (.png, .jpg, .jpeg, .gif, .bmp, .tif, .tiff, .webp) — Docling image format intentionally excluded for now
  • .ppt / .xls (legacy Office)
  • .odt / .ods / .odp
  • .rtf
  • .zip and other archives
  • Any other extension not in the supported list above

Google Workspace native docs (application/vnd.google-apps.*) are exported as PDF and treated as supported.

Example SourceCrawler status

status:
  conditions:
    - type: SourceCrawlerReady
      status: "True"
      reason: SuccessfullyReconciled
      message: >-
        successfully reconciled source crawler: my-pipeline-crawl;
        skipped 2 unsupported file(s): documents/photo.png, documents/notes.txt

Crawl catalog output

Path:

pipelines/<pipeline>/stages/crawl/catalog/<file_id>.json

Successful file:

{
  "file_id": "1Fen8XcdmzOVM8pY3wG8b-rTc-llm39z0",
  "file_name": "Data and AI Sprint Review ... Notes by Gemini.pdf",
  "media_type": "application/pdf",
  "extension": "pdf",
  "status": "successful",
  "source_type": "googleDrive",
  "crawled_at": "2026-08-03T06:14:32Z"
}

Skipped unsupported type:

{
  "file_id": "...",
  "file_name": "notes.txt",
  "extension": "txt",
  "status": "skipped",
  "reason": "unsupported file type \".txt\"",
  "source_type": "s3",
  "crawled_at": "2026-08-03T06:14:32Z"
}
Outcome status reason
Supported sync OK successful
Unsupported type skipped unsupported file type ".txt"
Oversize (>128MB) skipped file exceeds max size limit (128 MB)
Store/download fail error error text
Folder no catalog row

Summary by CodeRabbit

New Features

  • Added crawl tracking for files discovered during S3 and Google Drive synchronization.
  • Unsupported and oversized files are recorded and excluded from processing.
  • Added support for common document formats, including DOC/DOCX, PPTX, HTML, PDF, Markdown, CSV, XLSX, and AsciiDoc.
  • Crawl results include file status, source details, file type, reason, and timestamp.
  • Stale crawl records are automatically cleaned up.

Bug Fixes

  • Successful synchronization messages now identify skipped unsupported files, including additional skipped-file counts.

@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@PuneetPunamiya
PuneetPunamiya force-pushed the skip-unsupported-file-types branch from 158b794 to 5c95467 Compare July 31, 2026 07:27
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds supported file-type helpers and a crawl catalog. S3 and Google Drive synchronization now records file outcomes, skips unsupported and oversized files, removes stale catalog entries, and reports skipped unsupported files during reconciliation.

Changes

Crawl Catalog and Unsupported File Handling

Layer / File(s) Summary
File type and crawl catalog contracts
pkg/unstructured/file_types.go, pkg/unstructured/crawl_result.go
Defines supported extensions, file-type checks, crawl statuses, the CrawlResult JSON shape, catalog paths, extension normalization, and persistence.
Source synchronization and catalog lifecycle
pkg/unstructured/source.go, test/e2e/unstructured_test.go
S3 and Google Drive sources record skipped, successful, and failed files. They track unsupported files, classify oversized downloads, remove stale catalog entries, and update source-count and hash test fixtures.
Skipped-file reconciliation reporting
internal/controller/sourcecrawler_controller.go
Successful status messages report skipped-file counts and names. Displayed names are limited to ten, with an additional-count suffix when needed.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant S3BucketSource
  participant GDriveSource
  participant FileStore
  S3BucketSource->>S3BucketSource: classify discovered objects
  S3BucketSource->>FileStore: persist CrawlResult records
  GDriveSource->>GDriveSource: filter and download Drive files
  GDriveSource->>FileStore: persist CrawlResult records
  S3BucketSource->>FileStore: remove stale catalog entries
  GDriveSource->>FileStore: remove stale catalog entries
Loading

Possibly related PRs

Suggested reviewers: concaf

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: skipping unsupported file types during source crawling.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
pkg/unstructured/file_types.go (1)

26-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider making the extension set immutable to callers.

SupportedFileExtensions is an exported map[string]bool. Any package that imports unstructured can add or remove entries at runtime, for example unstructured.SupportedFileExtensions[".txt"] = true. This would change filtering behavior for all S3BucketSource and GDriveSource instances without going through IsSupportedFileType.

Unexport the map and keep only IsSupportedFileType and FileExtension as the public surface, or expose a copy through an accessor function.

♻️ Proposed fix
-// SupportedFileExtensions mirrors Docling defaultFromFormats, excluding image.
-// Supported: docx, pptx, html, pdf, asciidoc, md, csv, xlsx.
-var SupportedFileExtensions = map[string]bool{
+// supportedFileExtensions mirrors Docling defaultFromFormats, excluding image.
+// Supported: docx, pptx, html, pdf, asciidoc, md, csv, xlsx.
+var supportedFileExtensions = map[string]bool{
 	".docx":     true,
 	".pptx":     true,
 	".html":     true,
 	".htm":      true,
 	".pdf":      true,
 	".adoc":     true,
 	".asciidoc": true,
 	".md":       true,
 	".markdown": true,
 	".csv":      true,
 	".xlsx":     true,
 }

 func IsSupportedFileType(fileName string) bool {
-	return SupportedFileExtensions[strings.ToLower(path.Ext(fileName))]
+	return supportedFileExtensions[strings.ToLower(path.Ext(fileName))]
+}
+
+// IsSupportedExtension checks a pre-computed, normalized extension (e.g. an
+// extension overridden for Google-native document conversion).
+func IsSupportedExtension(ext string) bool {
+	return supportedFileExtensions[strings.ToLower(ext)]
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/unstructured/file_types.go` around lines 26 - 38, Make the supported
extension collection private instead of exposing the mutable
SupportedFileExtensions map. Update IsSupportedFileType and FileExtension to use
the private collection, preserving their existing public behavior; if callers
require access, provide a copy-returning accessor rather than the underlying
map.
internal/controller/sourcecrawler_controller.go (1)

315-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider moving skipped-file reporting onto the DataSource interface.

skippedUnsupportedFromSource type-switches on *unstructured.S3BucketSource and *unstructured.GDriveSource. This works today because Reconcile only constructs these two types. If a future DataSource implementation is added and this switch is not updated, its skipped files silently disappear from the status message with no compile-time warning.

Add a method to the DataSource interface in pkg/unstructured/source.go instead, so every implementation must supply its own skipped-file list.

♻️ Proposed fix
 type DataSource interface {
 	// SyncFilesToFilestore will store all files from the source to the filestore and return the list of file paths
 	SyncFilesToFilestore(ctx context.Context, fs *filestore.FileStore) ([]RawFileMetadata, error)
+	// SkippedUnsupportedFiles returns file paths skipped during the last sync
+	// due to an unsupported extension.
+	SkippedUnsupportedFiles() []string
 }
-func skippedUnsupportedFromSource(source unstructured.DataSource) []string {
-	switch s := source.(type) {
-	case *unstructured.S3BucketSource:
-		return s.SkippedUnsupported
-	case *unstructured.GDriveSource:
-		return s.SkippedUnsupported
-	default:
-		return nil
-	}
-}
+func skippedUnsupportedFromSource(source unstructured.DataSource) []string {
+	return source.SkippedUnsupportedFiles()
+}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/sourcecrawler_controller.go` around lines 315 - 324, Move
skipped-file reporting into the DataSource interface by adding a method that
returns the source’s skipped unsupported files, and implement it for
S3BucketSource and GDriveSource. Update skippedUnsupportedFromSource to call
that interface method directly and remove its concrete-type switch, ensuring
future DataSource implementations must provide the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/controller/sourcecrawler_controller.go`:
- Around line 315-324: Move skipped-file reporting into the DataSource interface
by adding a method that returns the source’s skipped unsupported files, and
implement it for S3BucketSource and GDriveSource. Update
skippedUnsupportedFromSource to call that interface method directly and remove
its concrete-type switch, ensuring future DataSource implementations must
provide the behavior.

In `@pkg/unstructured/file_types.go`:
- Around line 26-38: Make the supported extension collection private instead of
exposing the mutable SupportedFileExtensions map. Update IsSupportedFileType and
FileExtension to use the private collection, preserving their existing public
behavior; if callers require access, provide a copy-returning accessor rather
than the underlying map.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Enterprise

Run ID: cbf8ee34-b863-44a0-b8a5-15ccce6fe5e8

📥 Commits

Reviewing files that changed from the base of the PR and between 7abca92 and 5c95467.

📒 Files selected for processing (3)
  • internal/controller/sourcecrawler_controller.go
  • pkg/unstructured/file_types.go
  • pkg/unstructured/source.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/unstructured/source.go`:
- Around line 78-80: Update the catalog identifier generation in the source
discovery flow around rel and fileID so distinct S3 keys always produce
distinct, path-safe identifiers. Replace the slash-to-double-underscore mapping
with a reversible full-key encoding or collision-resistant identifier, and use
the same identifier consistently for catalogIDs and garbage-collection tracking.
- Around line 87-93: Update the reconciliation flow in
pkg/unstructured/source.go so every listed catalog write or deletion failure is
added to the synchronization error collection in addition to being logged, then
return a non-nil reconciliation error after processing all items. Apply this to
lines 87-93, 103-110, 131-136, 362-369, 409-418, 453-460, and 486-495; preserve
mutex protection for the Google Drive download-result path at lines 453-460.
- Around line 147-158: Restrict the catalog GC branch around
CrawlCatalogPath-generated entries so raw objects such as catalog/report.pdf are
not treated as catalog JSON files. Use an isolated catalog namespace or validate
the path against the exact format produced by CrawlCatalogPath before deriving
catalogID and deleting; leave raw files and their sidecars to the normal GC
path.
- Line 357: Update the discovered-file handling around the catalogIDs assignment
to retain the shortcut target’s FileID even when the target is inaccessible, and
persist an error catalog result for that failed file. Ensure the failed entry is
included in catalogIDs so cleanup does not remove its previous catalog record.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Enterprise

Run ID: 59f6e081-21c6-4c99-92cb-16197305cc1f

📥 Commits

Reviewing files that changed from the base of the PR and between 5c95467 and b2bff5c.

📒 Files selected for processing (2)
  • pkg/unstructured/crawl_result.go
  • pkg/unstructured/source.go

Comment thread pkg/unstructured/source.go
Comment thread pkg/unstructured/source.go
Comment thread pkg/unstructured/source.go
Comment thread pkg/unstructured/source.go
@abhisheksgumadi

abhisheksgumadi commented Aug 3, 2026

Copy link
Copy Markdown

Sorry, just to clarify why is .txt, .doc not supported for example? We have done work before to support these files inside Docling. @maharora has done work before in Analyze to support these formats inside Docling. There is already code written before to support this. Can we not take that please?

@PuneetPunamiya

Copy link
Copy Markdown
Contributor Author

Sorry, just to clarify why is .txt, .doc not supported for example? We have done work before to support these files inside Docling. @maharora has done work before in Analyze to support these formats inside Docling. There is already code written before to support this. Can we not take that please?

Yeah sure will update the patch and include the changes, thanks for the review 🤗

Comment thread pkg/unstructured/crawl_result.go Outdated
Comment thread internal/controller/sourcecrawler_controller.go Outdated
@PuneetPunamiya
PuneetPunamiya force-pushed the skip-unsupported-file-types branch from b2bff5c to 0d54c2f Compare August 4, 2026 06:40

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/unstructured/crawl_result.go`:
- Line 34: Update the CrawlMetadataDir constant and the related catalog-entry
path construction to use the catalog directory under the crawl output path,
producing stages/&lt;crawl&gt;/catalog/&lt;fileID&gt;.json so reconciliation and
stale-entry cleanup resolve the same location.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Enterprise

Run ID: 89af6d8c-9505-4856-9cd9-6b969e52b0d8

📥 Commits

Reviewing files that changed from the base of the PR and between b2bff5c and 0d54c2f.

📒 Files selected for processing (5)
  • internal/controller/sourcecrawler_controller.go
  • pkg/unstructured/crawl_result.go
  • pkg/unstructured/file_types.go
  • pkg/unstructured/source.go
  • test/e2e/unstructured_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/controller/sourcecrawler_controller.go
  • pkg/unstructured/source.go

Comment thread pkg/unstructured/crawl_result.go
@PuneetPunamiya
PuneetPunamiya force-pushed the skip-unsupported-file-types branch from 0d54c2f to 352eaaa Compare August 4, 2026 08:45

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
pkg/unstructured/source.go (2)

481-496: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Remove stale content when a file becomes oversized.

Line 481 adds the file to currentFiles before download. If storeFile later returns errFileExceedsMaxSize, garbage collection retains the previous raw file, metadata, and permissions. The catalog reports skipped, but downstream processing can still use stale content.

Remove the file ID from currentFiles under mu when this sentinel occurs.

Proposed fix
 if errors.Is(err, errFileExceedsMaxSize) {
+    mu.Lock()
+    delete(currentFiles, record.FileID)
+    mu.Unlock()
     logger.Info("WARNING: skipping file exceeding max file size limit",
         "fileID", record.FileID, "fileName", record.FileName)
     status, reason = CrawlStatusSkipped, err.Error()
 }

Also applies to: 525-526, 590-593

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/unstructured/source.go` around lines 481 - 496, In the download error
handling around storeFile, remove record.FileID from currentFiles while holding
mu whenever the error matches errFileExceedsMaxSize. Apply the same cleanup to
the corresponding oversized-file branches at the other reported locations, while
preserving the existing skipped status and reason handling.

328-331: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Abort garbage collection after a folder crawl failure.

filterCrawlRecords logs r.err and removes that folder from currentFiles and catalogIDs. Reconciliation then runs garbageCollect, which deletes the stored files, permissions, and catalog entries for the failed folder. It finally reports success.

Return the crawl errors to SyncFilesToFilestore. If any folder crawl fails, return an error before garbage collection.

Also applies to: 426-436, 525-526

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/unstructured/source.go` around lines 328 - 331, Update filterCrawlRecords
to propagate folder crawl failures instead of only logging and removing the
failed folder’s records. Have SyncFilesToFilestore detect and return the crawl
error before invoking garbageCollect, preserving the failed folder’s stored
files, permissions, and catalog entries and avoiding a successful reconciliation
result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@pkg/unstructured/source.go`:
- Around line 481-496: In the download error handling around storeFile, remove
record.FileID from currentFiles while holding mu whenever the error matches
errFileExceedsMaxSize. Apply the same cleanup to the corresponding
oversized-file branches at the other reported locations, while preserving the
existing skipped status and reason handling.
- Around line 328-331: Update filterCrawlRecords to propagate folder crawl
failures instead of only logging and removing the failed folder’s records. Have
SyncFilesToFilestore detect and return the crawl error before invoking
garbageCollect, preserving the failed folder’s stored files, permissions, and
catalog entries and avoiding a successful reconciliation result.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Enterprise

Run ID: ec26cc73-46c6-40db-97c4-3ffe61feb2b0

📥 Commits

Reviewing files that changed from the base of the PR and between 0d54c2f and 352eaaa.

📒 Files selected for processing (5)
  • internal/controller/sourcecrawler_controller.go
  • pkg/unstructured/crawl_result.go
  • pkg/unstructured/file_types.go
  • pkg/unstructured/source.go
  • test/e2e/unstructured_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/unstructured/file_types.go
  • internal/controller/sourcecrawler_controller.go

@PuneetPunamiya
PuneetPunamiya force-pushed the skip-unsupported-file-types branch from 352eaaa to b79544f Compare August 4, 2026 10:59

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/unstructured/source.go (1)

481-496: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Remove stale content when a Google Drive file becomes oversized.

currentFiles includes the file before storeFile checks its downloaded size. If an existing file later grows beyond 128 MB, storeFile returns the sentinel without replacing the old content. Garbage collection then preserves that old content because its file ID remains in currentFiles.

Remove the raw file and metadata for this skipped result, or exclude the file ID from currentFiles so garbage collection removes them.

Also applies to: 525-526

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/unstructured/source.go` around lines 481 - 496, Update the oversized-file
branch in the dlGroup callback around storeFile so a CrawlStatusSkipped result
for errFileExceedsMaxSize does not remain represented in currentFiles. Remove
the existing raw file and associated metadata, or exclude record.FileID from
currentFiles before garbage collection, while preserving the current warning and
skipped status behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/unstructured/source.go`:
- Around line 319-322: Update GDriveSource.filterCrawlRecords to return the
encountered folder crawl errors alongside the filtered records and inventory,
rather than only logging them. Propagate those errors to the caller at the
reconciliation logic around line 526, and when any folder crawl fails, return a
reconciliation error before garbage collection so missing records are not
treated as deleted.
- Around line 84-94: The current S3 size check relies on listing metadata;
enforce the limit in the object-read flow instead. Update the GetObject/read
handling to cap reads at maxFileSize+1, detect content exceeding maxFileSize,
and return errFileExceedsMaxSize before invoking fs.Store; retain the existing
crawl-result handling for skipped oversized files.

In `@test/e2e/unstructured_test.go`:
- Around line 493-494: Update the unstructured destination test around
foundCount to derive expected files from supported, non-oversized inputs, then
assert each expected file has a destination result instead of only checking that
foundCount is nonzero. Also validate skipped files using their crawl catalog
entries, reusing the outcome data recorded by the production source flow.
- Around line 503-509: Update the hash-test cleanup in the surrounding
unstructured test to delete the bucket entry keyed by pdflatex-4-pages.pdf
instead of removing filesInBucket[0]. Preserve pdflatex-outline.pdf so the later
upload exercises a new file without overwriting the wrong fixture.

---

Outside diff comments:
In `@pkg/unstructured/source.go`:
- Around line 481-496: Update the oversized-file branch in the dlGroup callback
around storeFile so a CrawlStatusSkipped result for errFileExceedsMaxSize does
not remain represented in currentFiles. Remove the existing raw file and
associated metadata, or exclude record.FileID from currentFiles before garbage
collection, while preserving the current warning and skipped status behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Enterprise

Run ID: a2d5a0a9-4db5-47d7-ada5-f8aeaf32c3b6

📥 Commits

Reviewing files that changed from the base of the PR and between 352eaaa and b79544f.

📒 Files selected for processing (5)
  • internal/controller/sourcecrawler_controller.go
  • pkg/unstructured/crawl_result.go
  • pkg/unstructured/file_types.go
  • pkg/unstructured/source.go
  • test/e2e/unstructured_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/unstructured/file_types.go
  • internal/controller/sourcecrawler_controller.go

Comment on lines 84 to +94
if object.Size != nil && *object.Size > maxFileSize {
logger.Info("WARNING: skipping file exceeding max file size limit",
"key", *object.Key, "sizeMB", *object.Size/(1<<20))
if err := storeCrawlResult(ctx, fs, outputPrefix, CrawlResult{
FileID: fileID, FileName: baseName, SourcePath: sourcePath,
Status: CrawlStatusSkipped, Reason: "file exceeds max size limit (128 MB)",
SourceType: "s3",
}); err != nil {
logger.Error(err, "failed to store crawl catalog result", "fileID", fileID)
}
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Enforce the S3 size limit while reading the object.

The listing size is not a reliable enforcement point. The object can change between ListObjectsInPrefix and GetObject, and object.Size can be absent.

Limit the read to maxFileSize+1. Return errFileExceedsMaxSize before fs.Store if the downloaded content exceeds the limit.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/unstructured/source.go` around lines 84 - 94, The current S3 size check
relies on listing metadata; enforce the limit in the object-read flow instead.
Update the GetObject/read handling to cap reads at maxFileSize+1, detect content
exceeding maxFileSize, and return errFileExceedsMaxSize before invoking
fs.Store; retain the existing crawl-result handling for skipped oversized files.

Comment on lines +319 to +322
func (g *GDriveSource) filterCrawlRecords(
ctx context.Context, fs *filestore.FileStore,
results []folderResult,
) ([]gdrive.CrawlRecord, map[string]bool) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Do not run garbage collection after a partial folder crawl.

filterCrawlRecords logs folderResult.err and returns a partial inventory. Line 526 then treats missing records as deleted source files and removes their stored files, permissions, and catalog entries.

Return the folder crawl errors from filterCrawlRecords. If any folder crawl fails, return a reconciliation error and skip garbage collection.

Also applies to: 436-436, 525-526

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/unstructured/source.go` around lines 319 - 322, Update
GDriveSource.filterCrawlRecords to return the encountered folder crawl errors
alongside the filtered records and inventory, rather than only logging them.
Propagate those errors to the caller at the reconciliation logic around line
526, and when any folder crawl fails, return a reconciliation error before
garbage collection so missing records are not treated as deleted.

Comment thread test/e2e/unstructured_test.go
Comment on lines +503 to +509
file2Content, err := os.ReadFile(filepath.Join(unstructuredFilesDirectory, "pdflatex-4-pages.pdf"))
if err != nil {
t.Fatalf("read file2 test PDF: %v", err)
}

file1 := "pdflatex-4-pages.pdf"
hashTestFile2Key := fmt.Sprintf("%s/pdflatex-outline.pdf", dataPipelineCRName)
file1 := "pdflatex-outline.pdf"
hashTestFile2Key := fmt.Sprintf("%s/pdflatex-4-pages.pdf", dataPipelineCRName)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

mapfile -t names < <(
  fd --type f --max-depth 1 . test/resources/unstructured/unstructured-files |
    sed 's#^.*/##' |
    sort
)

printf '%s\n' "${names[@]}"

[[ "${names[0]:-}" == "pdflatex-4-pages.pdf" ]] || {
  echo "The first fixture is not pdflatex-4-pages.pdf" >&2
  exit 1
}

printf '%s\n' "${names[@]}" | grep -Fxq "pdflatex-outline.pdf"

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 237


🏁 Script executed:

set -euo pipefail

printf '%s\n' '--- fixture files ---'
python3 - <<'PY'
from pathlib import Path
root = Path("test/resources/unstructured/unstructured-files")
for p in sorted(root.iterdir(), key=lambda p: p.name):
    if p.is_file():
        print(p.name)
PY

printf '%s\n' '--- test context ---'
sed -n '450,590p' test/e2e/unstructured_test.go

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 5298


🏁 Script executed:

set -euo pipefail

printf '%s\n' '--- filesInBucket references ---'
rg -n -C 8 'filesInBucket|unstructuredFilesDirectory|PutObject' test/e2e/unstructured_test.go

printf '%s\n' '--- fixture construction and deletion context ---'
sed -n '250,455p' test/e2e/unstructured_test.go

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 14808


Delete the hash-test file explicitly.

The fixture set includes minimal-document.pdf, pdflatex-4-pages.pdf, and pdflatex-outline.pdf. Deleting filesInBucket[0] removes minimal-document.pdf, so the later upload overwrites an existing file instead of testing a new file. Delete pdflatex-4-pages.pdf by key and retain pdflatex-outline.pdf.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/unstructured_test.go` around lines 503 - 509, Update the hash-test
cleanup in the surrounding unstructured test to delete the bucket entry keyed by
pdflatex-4-pages.pdf instead of removing filesInBucket[0]. Preserve
pdflatex-outline.pdf so the later upload exercises a new file without
overwriting the wrong fixture.

return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("unsupported source type: %s", sourceCrawlerConfig.Type))
}

storedFiles, err := source.SyncFilesToFilestore(ctx, r.fileStore)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one approach is that we give this information to the source from source crawler because this is a property of the source crawler and not of the individual source like Google Drive or S3

Only sync Docling-compatible document extensions from S3/GDrive and
surface skipped files in the SourceCrawler Ready status message.
Persist per-file crawl outcomes under stages/<crawl>/catalog/ so
success, skipped, and error results can be synced and queried later.
@PuneetPunamiya
PuneetPunamiya force-pushed the skip-unsupported-file-types branch from b79544f to 6a88cf1 Compare August 6, 2026 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants